Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[CLI] MACsec support #1727

Open
wants to merge 4 commits into
base: master
Choose a base branch
from
Open

[CLI] MACsec support #1727

wants to merge 4 commits into from

Conversation

qbdwlr
Copy link
Contributor

@qbdwlr qbdwlr commented Jul 26, 2021

What I did

  1. Added basic config add/del for MACsec port
  2. Added config add/del for MACsec profile (config MACsec CLI #1895 with bug fixes)
  3. Added "show macsec connections" command
  4. Added "show macsec statistics" command

How I did it

How to verify it

System integration test of port add/del and repeated port add/del/add testing on JNPR and VS MACsec platforms.
System integration test of profile add/del on JNPR and VS MACsec platforms.
System integration test of "show macsec connections" on JNPR and VS MACsec platforms.
System integration test of "show macsec statistics" on JNPR and VS MACsec platforms.

Previous command output (if the output of a command-line utility has changed)

N/A

New command output (if the output of a command-line utility has changed)

N/A

@lgtm-com
Copy link

lgtm-com bot commented Jul 26, 2021

This pull request introduces 1 alert when merging 766aaf9 into 4422911 - view on LGTM.com

new alerts:

  • 1 for Unused import

@Pterosaur
Copy link
Contributor

Please fix the lgtm alert

@lgtm-com
Copy link

lgtm-com bot commented Nov 1, 2021

This pull request introduces 1 alert when merging 42eab9a8aa36d6dfaa8c430fe18fa08d324aec82 into ca728b8 - view on LGTM.com

new alerts:

  • 1 for Unused import

@lgtm-com
Copy link

lgtm-com bot commented Nov 2, 2021

This pull request introduces 4 alerts when merging c5ed372 into ca728b8 - view on LGTM.com

new alerts:

  • 2 for Unused local variable
  • 2 for Unused import

@Pterosaur Pterosaur self-requested a review November 2, 2021 01:01
@lgtm-com
Copy link

lgtm-com bot commented Nov 5, 2021

This pull request introduces 4 alerts when merging 193ef4c53f31982953f3b4a911871bf6f8ab1910 into 095bf54 - view on LGTM.com

new alerts:

  • 2 for Unused local variable
  • 2 for Unused import

Pterosaur
Pterosaur previously approved these changes Nov 8, 2021
Comment on lines +73 to +85
def display_connections(self, intf_name):
"""
Get MACsec info from wpa_cli.
"""
macsec_cmd = 'docker exec -it macsec wpa_cli -g /var/run/' + intf_name + ' IFNAME=' + intf_name + ' macsec'
click.echo("Interface: {} ".format(intf_name))
p = subprocess.Popen(macsec_cmd, stdout=subprocess.PIPE, stderr=subprocess.PIPE, shell=True)
(output, err) = p.communicate()
rc = p.wait()
if rc == 0:
click.echo(output)
else:
click.echo("Error retrieving MACsec connections: {} ".format(err))
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think it's better to fetch the connections from APP DB instead of wpa_cli. Because in the future, maybe we will use multiple wpa_supplicant processes to one port for Primary/Fallback key feature. Or we can use the static SAK directly in APP DB for debugging.
So, I think it's better to believe the data in APP DB for more flexibility.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This PR is here for contribution to the community in its current state. Only bug fixes will be made to this PR at this point. Any major functionality changes will not be made in the context of this PR. If a different implementation of the "show macsec connections" command is preferred, feel free to use the changes in this PR as a basis for another PR and I will close this one.

@qbdwlr qbdwlr marked this pull request as ready for review November 9, 2021 17:16
@qbdwlr
Copy link
Contributor Author

qbdwlr commented Nov 9, 2021

/azpw run

@mssonicbld
Copy link
Collaborator

/AzurePipelines run

@azure-pipelines
Copy link

Azure Pipelines successfully started running 1 pipeline(s).

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants